Skip to content

fix(exposition): export internal package for OSGi resolution - #2415

Open
arimu1 wants to merge 5 commits into
prometheus:mainfrom
arimu1:fix/2395-osgi-expositionformats-internal
Open

fix(exposition): export internal package for OSGi resolution#2415
arimu1 wants to merge 5 commits into
prometheus:mainfrom
arimu1:fix/2395-osgi-expositionformats-internal

Conversation

@arimu1

@arimu1 arimu1 commented Aug 24, 2026 •
edited
Loading

Copy link
Copy Markdown

Fixes #2395

Summary

  • Export io.prometheus.metrics.expositionformats.internal from both prometheus-metrics-exposition-formats and prometheus-metrics-exposition-formats-no-protobuf OSGi bundles
  • prometheus-metrics-exposition-textformats imports that package because PrometheusProtobufWriter loads PrometheusProtobufWriterImpl via Class.forName, but bnd kept the implementation package private in the formats bundles, so OSGi resolution failed

Approach

Used bnd _exportcontents to export the otherwise-private internal package while preserving existing generated* exports. This matches the metadata wiring fix suggested in the issue (option 1) without moving implementation classes across bundle boundaries.

Prove-it (before fix on HEAD)

prometheus-metrics-exposition-textformats
  Import-Package: ... io.prometheus.metrics.expositionformats.internal ...

prometheus-metrics-exposition-formats
  Export-Package: io.prometheus.metrics.expositionformats.generated*, ...
  (internal not exported)

After fix

Both formats bundles now export io.prometheus.metrics.expositionformats.internal, satisfying the textformats import.

Test plan

  • mvn -pl prometheus-metrics-exposition-formats,prometheus-metrics-exposition-formats-shaded,prometheus-metrics-exposition-textformats -am test (Java 25)
  • Inspected META-INF/MANIFEST.MF in built JARs to confirm internal package is exported and generated exports remain

@arimu1
fix(exposition): export internal package for OSGi resolution
687e885 
prometheus-metrics-exposition-textformats imports
io.prometheus.metrics.expositionformats.internal because
PrometheusProtobufWriter loads the protobuf implementation via
Class.forName, but the exposition-formats bundles kept that package
private. Export it via bnd _exportcontents so OSGi can wire the two
bundles together.

Signed-off-by: arimu1 <19286898+arimu1@users.noreply.github.com>
@jaydeluca
jaydeluca requested a lite review from Copilot August 24, 2026 12:49
Copilot AI reviewed Aug 24, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes OSGi bundle resolution for prometheus-metrics-exposition-textformats by ensuring the implementation package io.prometheus.metrics.expositionformats.internal is exported from the exposition-formats bundles, matching the wiring expected by PrometheusProtobufWriter’s reflective Class.forName loading approach.

Changes:

  • Adds bnd (maven-bundle-plugin) instructions to export io.prometheus.metrics.expositionformats.internal via _exportcontents for OSGi wiring.
  • Keeps io.prometheus.metrics.expositionformats.generated* exported explicitly to preserve existing generated exports.
  • Applies the same export behavior to both the “formats” and “no-protobuf” bundle-producing modules.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
prometheus-metrics-exposition-formats/pom.xml Exports the internal implementation package for OSGi resolution while preserving generated package exports.
prometheus-metrics-exposition-formats-shaded/pom.xml Mirrors the same OSGi export instructions for the shaded formats bundle so consumers can resolve the internal package consistently.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

jaydeluca
jaydeluca reviewed Aug 24, 2026
View reviewed changes

@jaydeluca jaydeluca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not super familiar with OSGI, so I would like to see some tests or other evidence that this solves the issue if possible

Comment thread prometheus-metrics-exposition-formats-shaded/pom.xml
Comment thread prometheus-metrics-exposition-formats-shaded/pom.xml
Comment thread prometheus-metrics-exposition-formats/pom.xml Outdated
@arimu1
test(exposition): assert OSGi export of internal package
bc2860e 
Lock the formats bundle Export-Package and textformats Import-Package
headers so OSGi wiring of expositionformats.internal is regression-tested.

Signed-off-by: arimu1 <19286898+arimu1@users.noreply.github.com>
@arimu1

arimu1 commented Aug 26, 2026

Copy link
Copy Markdown
Author

Added OsgiBundleManifestTest (runs in both exposition-formats bundles) that reads the bnd-generated MANIFEST.MF and asserts:

  • formats Export-Package includes io.prometheus.metrics.expositionformats.internal
  • textformats Import-Package includes the same package

Local Temurin 25: OsgiBundleManifestTest 2/2 in both formats modules; exposition tests 107 (textformats) + 68 (formats) + 68 (shaded).

@arimu1
fix(exposition): drop SNAPSHOT OSGi versions and optional import
0e7138a 
Let bnd emit 1.8.1 package versions like sibling bundles. Exclude the
phantom protobuf import from the shaded formats bundle, and mark the
textformats internal import optional so a textformats-only install still
resolves.

Signed-off-by: arimu1 <19286898+arimu1@users.noreply.github.com>
zeitlinger
zeitlinger approved these changes Aug 26, 2026
View reviewed changes

@zeitlinger zeitlinger left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed latest head: CI is green, no merge conflicts, and no blocking review comments remain.

@zeitlinger

zeitlinger commented Aug 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Thanks for the contribution!

@zeitlinger
zeitlinger enabled auto-merge (squash) August 26, 2026 09:26
@zeitlinger

zeitlinger commented Aug 26, 2026

Copy link
Copy Markdown
Member

@arimu1 please fix the format error

@arimu1
style(exposition): google-java-format OsgiBundleManifestTest
0b37914 
Keep INTERNAL_PACKAGE on one line so flint google-java-format 1.36.1 passes.

Signed-off-by: arimu1 <19286898+arimu1@users.noreply.github.com>
auto-merge was automatically disabled August 27, 2026 07:01

Head branch was pushed to by a user without write access

@arimu1

arimu1 commented Aug 27, 2026

Copy link
Copy Markdown
Author

Fixed the lint failure: google-java-format 1.36.1 wanted INTERNAL_PACKAGE on one line in OsgiBundleManifestTest. flint/GJF dry-run is clean locally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jaydeluca jaydeluca jaydeluca left review comments
Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@zeitlinger zeitlinger zeitlinger approved these changes
@fstab fstab Awaiting requested review from fstab fstab is a code owner
@dhoard dhoard Awaiting requested review from dhoard dhoard is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

expositionformats.internal is imported by exposition-textformats but exported by nothing, breaking OSGi resolution

4 participants

@arimu1 @zeitlinger @jaydeluca